Fix/adfa 4514 [KDoc-to-JSON plugin] - #18
Conversation
configJson was a dedicated Json instance used only for the manual config-decode fallback path. Inlining it removes the standalone property while keeping the same ignoreUnknownKeys tolerance for extra keys in user-authored plugin config. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- scripts/verify_package_index.py: confirms every object listed in a package's index.json actually has documented content on the page its url points to (matching by dri), not just that the file exists. - scripts/kotlin/test_kotlin_stdlib.sh: pure-bash check that the default HTML build and the kdoc-to-json JSON build have a one-to-one set of pages (extension-swap aware, in both directions). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
From Claude AI: Issues found
kotlin Every other field in this data class relies on its default (property-name-matching) serial name — this is the only one with a custom @SerialName, and it maps to kebab-case "omit-nulls" instead of "omitNulls". But the README's own config example (section 3) and its config table both show: json Since parsing goes through kotlinx.serialization either via Dokka's native config decoder or the manual Json { ignoreUnknownKeys = true } fallback in JsonRenderer.kt, a user who copies the README's example verbatim will have "omitNulls" silently treated as an unknown key, and omitNulls will stay at its default false — with no error, since ignoreUnknownKeys swallows it. This seems like a stale/accidental annotation (every other field name should probably work here) rather than an intentional key rename. Worth fixing the annotation (or the docs, if kebab-case really is intended) and probably adding a quick round-trip test on JsonPluginConfig to catch this class of bug in future.
kotlin Every unresolved: gets rewritten in this same pass — resolved ones become a relative path, unresolved ones become a bare "#". So the final on-disk JSON never actually contains an unresolved: string to grep for; the documented troubleshooting technique (search rendered JSON for unresolved:) won't find anything even when links failed to resolve. If diagnosing failed resolutions is meant to be possible, the plugin needs to log which DRIs fell through to "#" (it currently doesn't — only a final aggregate "Successfully resolved $replacedCount..." count is logged, with no count/list of failures), or the docs should stop implying you can find them by inspecting output.
On the "sourceSet whitelisting" commits The last two commits (sourceSet whitelisting, Whitelist sourceSets per ADFA-4737) look purposeful and match a real ticket, and the whitelist logic itself (in JsonRenderer/ModelMapper) looks correct from what I read. I'd suggest double-checking verify_sourceset_whitelist.py gets run against a real kotlin-stdlib build with a whitelist configured, as part of this PR's test plan — I didn't see that confirmation anywhere in the PR description (there's no "Test plan" checklist here at all, unlike #21). Suggestion: given the size (10 commits, an entire new Gradle module) and no test plan in the description, it'd help reviewers if the PR description at least linked a sample run's output (e.g. from scripts/build-example.sh or test_kotlin_stdlib.sh) so they don't have to build the whole thing locally to sanity-check items 1–2 above. |
- Remove stray @SerialName("omit-nulls") on JsonPluginConfig.omitNulls
so the README's documented "omitNulls" key actually deserializes
instead of silently being ignored.
- Log DRIs that LinkPostProcessor fails to resolve (patched to "#"),
and wrap pass 2's file rewrite in try/catch so one bad file no
longer aborts link resolution for the rest of the build.
- Document the last-writer-wins behavior when a DRI resolves in more
than one file.
- Update README section 7 to match actual behavior: the final JSON
never contains "unresolved:" strings, so failures should be found
via the build log instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix examples/example-data-processor first: Nesting.kt referenced Meta, Provider, and DataMap, none of which were defined anywhere, so the example library didn't even compile. Add Types.kt with those definitions, which also happens to exercise the annotation/interface/ typeAlias "kind" values. Parameterize the example project's plugin config (build.gradle.kts) to read from a KDOC2JSON_TEST_CONFIG env var when set, so tests can drive the plugin through arbitrary configs without editing the build script. Add tests/, one bash script per config option (logLevel, logFile, replaceHtmlExtension, omitFields, omitNulls, classDiscriminator, prettyPrint, sourceSetWhitelist) plus a shared lib.sh harness and run_all.sh runner. Each test builds the example project against a real config and asserts on the resulting JSON output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed David's comment, added tests for config options, added a test plan I'm going to start working through |
Fills the fixture gaps listed in TEST_PLAN.md §1 so later steps (§2-§9) have real constructs to assert against: a bounded generic (BoundedContainer<T : Comparable<T>>), use-site variance (copyItems(List<out Number>, MutableList<in Number>)), a nullable type and Java interop (formatOrDefault, currentJavaDate), an extension function/property, a suspend function, an infix function, an operator function, a function with a default parameter, a repeatable multi-parameter annotation applied twice plus one applied to a parameter, a data class, a sealed class hierarchy with a real companion object, equals/hashCode/toString overrides, a Throwable subclass, and a KDoc comment exercising @param/@return/@throws/@see, two @sample tags, nested bold/italic/list markup, and raw <, >, & in prose. Verified each construct against the actual generated JSON (not just that it compiles) -- e.g. confirmed the "redundant projection" compiler warning on copyItems doesn't stop Dokka's model from still emitting distinct Covariance/Contravariance projections. Not included: the expect/actual fixture (TEST_PLAN §1 last row). That needs converting this example project to Kotlin Multiplatform with a second source set, which would invalidate the "main"-source-set assumption the existing tests/test_*.sh scripts rely on -- deferred to its own change rather than bundled in here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds tests/test_dto_mapping.sh, test_type_mapping.sh, and test_doc_tags.sh,
covering TEST_PLAN.md §2 (ModelMapper.mapToDto correctness), §3
(type/bound/projection mapping), and §4 (doc-tag & text extraction).
Structural assertions live in tests/helpers/check_*.py (with a small
shared checklib.py) since they need real JSON parsing rather than grep,
following the precedent already set by
scripts/verify_sourceset_whitelist.py.
Two more small fixture prerequisites, alongside step 1's Advanced.kt:
- Counter (a var property) and applyCallbacks (plain/extension/suspend
lambda parameters) in Advanced.kt, needed for the property
getter/setter and functional-type rows in §2/§3.
- A fenced code block and a blockquote added to safeDivide's KDoc, needed
for §4's block-level-tag row.
- dokkaSourceSets.configureEach { samples.from(...) } in build.gradle.kts:
without it Dokka never resolves @sample tags at all, regardless of the
plugin's own fallback logic.
Also fixes a real bug this surfaced in ModelMapper.kt: the @sample
fallback looked for a ContentCodeBlock whose *style* contained
"RunnableSample", but Dokka's DefaultSamplesTransformer actually tags
these blocks via ContentKind.Sample on `dci.kind`, not via style. The
old check silently matched nothing, so every @sample tag came through
with empty text. Verified via javap on dokka-base or the sample text is
now correctly extracted and multiple @sample tags on one doc each pull
their own distinct source.
Verified test efficacy by mutating a real mapping (Covariance ->
Contravariance) and confirming test_type_mapping.sh caught it, then
reverted.
Known gap, not fixed here (needs a schema change, not a test): `@see`
tags never get a resolved url. Dokka's `See` DocTag carries its
resolved link on a dedicated `address: DRI?` field, but TagWrapperDto
has no `url` property for mapDocNodes to populate. `@throws`/inline
[Foo]-links elsewhere still resolve correctly since those go through
the tag's `root` content instead.
Not covered: §4's per-source-set documentation row, which needs an
expect/actual pair across two source sets -- deferred along with the
rest of the multiplatform work noted in the Advanced.kt commit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds tests/test_renderer.sh (§6: package-list, all-types.json, breadcrumbs
at root/max-depth, and file-path parity vs Dokka's own HTML layout) and
tests/test_link_postprocessor.sh (§7: no lingering "unresolved:" strings,
and a genuinely-unresolvable DRI correctly patched to "#" with a matching
build-log warning). Structural checks live in tests/helpers/check_*.py,
same pattern as step 2.
New fixture for the file-path-parity check: examples/html-baseline, a
stock-Dokka-HTML sibling of example-data-processor that reuses the exact
same sources via kotlin.srcDir(...) but has no kdoc-to-json plugin
dependency, so it renders with Dokka's default HTML renderer. Confirmed
by diffing the two builds' page sets that our JSON pages line up with
Dokka's own HTML pages one-for-one, aside from each renderer's own
top-level artifacts (our all-types.json aggregate; Dokka's
navigation.html and static template assets) and a module-name-prefix
folder convention Dokka's default renderer adds that ours doesn't.
lib.sh: run_dokka now records the Gradle log path in LAST_GRADLE_LOG so
tests can assert on build-log content, not just written JSON.
Verified test efficacy by mutating a real behavior (package-list sort
order) and confirming test_renderer.sh caught it, then reverted.
Deferred (per discussion before starting this step): three TEST_PLAN
rows structurally need a real multi-module or multiplatform Dokka
build, which this single-module fixture can't provide -- confirmed
empirically ("Successfully resolved 0 cross-module links!" in the
build log, i.e. every "unresolved:" marker here is either resolved
directly by locationProvider or is permanently unresolvable):
- §6 multimodule index.json
- §7 relative-path-depth for a genuinely cross-module-resolved DRI
- §7 last-writer-wins for expect/actual
All three are left to TEST_PLAN.md §8's kotlin-stdlib stress test,
which already covers real multiplatform/multimodule behavior at scale.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds tests/test_robustness.sh covering: an unknown extra config key (tolerated), no plugin config registered at all (falls back to defaults), an empty/near-empty module (a package with zero public declarations doesn't crash the build), and a classDiscriminator collision (fails with a clear serialization error). New harness support in lib.sh: run_dokka_expect_failure (for assertions where the build SHOULD fail, without aborting the script) and run_dokka_no_plugin_config (drives the new KDOC2JSON_NO_PLUGIN_CONFIG env var in build.gradle.kts, which skips registering any pluginsConfiguration entry at all). New fixture: examples/example-data-processor/.../emptypkg/InternalOnly.kt, an internal-only declaration in its own package so that package has zero documented (public) members. Corrects a real inaccuracy this surfaced in TEST_PLAN.md's own "Malformed plugin config" row: the original wording assumed any bad config falls through to JsonRenderer's own try/catch and its lenient manual parse. Verified that's only true for valid-JSON-with-an-unknown-key. Genuinely invalid JSON syntax and valid JSON with a wrong-typed known field both crash the whole Gradle build one layer up -- Dokka's own Gradle plugin deserializes jsonEncode()'s output against JsonPluginConfig client-side (via Jackson) before our worker/renderer code ever runs, so those two cases never reach our try/catch at all. Nothing to fix in this plugin's code for that; it's upstream Dokka Gradle Plugin behavior. Updated the TEST_PLAN.md row to describe the actual, verified boundary instead. Verified test efficacy by mutating a real default (replaceHtmlExtension false -> true) and confirming test_robustness.sh caught it, then reverted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Implemented all automation specified in TEST_PLAN.md. Actual testing against Dokka base kotiln-stdlib can be done manually. |
luisguzman-adfa
left a comment
luisguzman-adfa
left a comment
There was a problem hiding this comment.
Hello @alexmmiller please find here the review I can provide;
PR #18 — review comments
Ran build-example.sh + run_all.sh on ARM64/Ubuntu 24.04: build OK, 14/14 test scripts pass.
The following comes from this review, each is a Suggestion (concrete fix) or a Decision point (design call to make). Most useful first:
1. Accessor/param links resolve to dead # (design call).
The example logged Failed to resolve 28 DRIs (patched to "#") — getters/setters, ctor params. resolveUrl (ModelMapper 24-40) emits unresolved: for synthetic accessors, then LinkPostProcessor (45-51) turns anything not in the index into "#".
Decision point: either resolve those DRIs to their owner's page (strip the getter/setter/PointingToCallableParameters suffix, look up the parent DRI) so they become real links, or document them as intentional non-links. In a 10-type fixture this already produced 28 dead links, so at real-library scale it could leave many references non-navigable.
2. href is not escaped — injection risk. extractText (ModelMapper 178-183) builds <a href="$href"> from KDoc; a " in a link breaks/injects into the HTML that ships inside the JSON.
Suggestion: Escape the attribute value (" → ").
3. Hard cast can crash mapping. mapProjection(...) as VarianceDto (ModelMapper 224) throws ClassCastException if Dokka returns Star/a bare bound.
Suggestion: as? VarianceDto ?: InvarianceDto(...).
4. Render loop isn't resilient like LinkPostProcessor now is. Page writes (JsonRenderer 178-207) have no try/catch, so one unwritable file aborts the whole build's output.
Suggestion: Wrap per-file write in try/catch + warn, matching the hardening already done in LinkPostProcessor.
5. mapExtras reflects on Dokka internals by class/method name (DefaultValue, AdditionalModifiers, ObviousMember…), swallowing failures into debug. This silently degrades when someone bumps Dokka (which the README invites).
Suggestion: Mark it version-coupled with a comment + add one assertion against the pinned Dokka so a break is loud.
6. omitNulls also strips empty strings/arrays/objects, not just nulls (JsonRenderer 247-252) — so functions: [] disappears.
Decision point: Note this in the README (consumers must treat missing keys as empty), or split it as omitEmpty.
7. README §7 is stale. It says grep the JSON for unresolved:, but nothing on disk contains it anymore — the info is in the Failed to resolve N DRIs log.
Suggestion: Point the doc at the log warning.
8. Preview-API warning at JsonOutputPlugin.kt:18
Suggestion: add @OptIn(DokkaPluginApiPreview::class).
9. Gradle drift (infra). Plugin uses wrapper 9.1.0, example uses 9.5.1 (two downloads); both warn they break on Gradle 10 (Kotlin 1.9.x plugin).
Suggestion: Align the wrappers; open a follow-up to bump the Kotlin plugin before Gradle 10.
PR description: add the test-plan and confirmation that verify_sourceset_whitelist.py was run against a real kotlin-stdlib build are not yet there — worth adding, along with the sample run (14/14)
1. Dead accessor/param links (ModelMapper.resolveUrl): getters/setters, constructors, and callable parameters never get their own PageNode, so they always fell through to "unresolved:" and got patched to a dead "#" by LinkPostProcessor -- 28 of them in this fixture alone. Now falls back to the owning declaration's own DRI (same package/class, no callable, PointingToDeclaration) so these resolve to a real, navigable page instead. Cuts this fixture's unresolved count from 28 to 1 (a genuinely pageless compiler-internal annotation with nothing to fall back to). Updated test_link_postprocessor.sh's "genuinely unresolvable DRI" case to that one remaining example. 2. href injection (ModelMapper.extractText): <a href="$href"> was built from raw KDoc/resolved-link content with no escaping. Added escapeHtmlAttribute (& before ", to avoid double-escaping a literal " already in the source) and applied it to both the raw-HTML-<a> and resolved-DocumentationLink cases. 3. Hard cast could crash mapping (ModelMapper): `mapProjection(...) as VarianceDto` for a type parameter's own variance would throw ClassCastException if Dokka ever handed back a Star/bare-Bound there. Added mapVariance(), which degrades to InvarianceDto around whatever bound can be salvaged instead of crashing. 4. Render loop wasn't resilient (JsonRenderer): wrapped the per-page write in try/catch + warn, matching the resilience LinkPostProcessor's own passes already have, so one unwritable/ unserializable page doesn't abort every other page's output. 5. mapExtras reflects on Dokka internals by class/method name and is silently fragile across Dokka version bumps. Marked it VERSION-COUPLED with a comment, and added DokkaVersionCheck, a one-time-per-process check that the exact FQCNs this reflection depends on are still loadable, logging a loud warning (not silently swallowed to debug) if a future Dokka bump renames/moves/removes one. 6. omitNulls also strips empty strings/arrays/objects, not just nulls. Documented this explicitly in the README (a missing key must be treated as its empty value) rather than splitting it into a new omitEmpty option, to avoid a config-surface change in this PR. 8. Added @OptIn(DokkaPluginApiPreview::class) on pluginApiPreviewAcknowledgement to silence the preview-API build warning. 9. Aligned kdoc-to-json's Gradle wrapper (was 9.1.0) with the example projects' (9.5.1) so a contributor building both doesn't trigger two separate wrapper downloads. Not done here (needs the user's own follow-up): opening a tracking issue for bumping the Kotlin Gradle plugin off 1.9.x before Gradle 10, and updating the PR description. Verified end to end: full local suite (tests/run_all.sh) still 14/14 after every change; a fresh dokkaGenerate log now shows only 1 "Failed to resolve" DRI instead of 28, and no preview-API warning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Plugin for Dokka that intercepts the HTML rendering process to output raw JSON data in its place.
Example output with sourceSet whitelisting: "kotlin-jvm-common-json.zip" attachment at https://appdevforall.atlassian.net/browse/ADFA-4737